Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: removing custom nth Zip fn #79173

Closed
wants to merge 5 commits into from
Closed

refactor: removing custom nth Zip fn #79173

wants to merge 5 commits into from

Conversation

DeveloperC286
Copy link
Contributor

@DeveloperC286 DeveloperC286 commented Nov 18, 2020

Noticed super_nth() that seems very similar to nth() in iterator.rs.

If you look at nth() in iterator.rs before the commit ecacc7534b6bf50205c37c89402565b82d95a257 super_nth() looks exactly the same as fn nth() in iterator.rs.

I may be misunderstanding something, but I think super_nth() can just be removed.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 18, 2020
@DeveloperC286
Copy link
Contributor Author

Not sure if someone can just kill the CI? It has been going for 1h 30mins and it usually consistency for others takes 40mins. I think my misunderstanding of traits/impl has caused an infinite loop maybe of nth() calling itself?

@jyn514 jyn514 added A-iterators Area: Iterators C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 18, 2020
@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 19, 2020
@kennytm
Copy link
Member

kennytm commented Nov 19, 2020

I think my misunderstanding of traits/impl has caused an infinite loop maybe of nth() calling itself?

Maybe let's figure out if this cleanup is really possible first (having the CI pass).

@DeveloperC286
Copy link
Contributor Author

@kennytm
I removed nth() and __iterator_get_unchecked() from impl<A: Iterator, B: Iterator> Iterator for Zip<A, B> as it is just the same as the default in iterator.rs. Although now impl<A: TrustedRandomAccess + Iterator, B: TrustedRandomAccess + Iterator> Iterator for Zip<A, B> which has the specialised implementations is complaining it can not create the specialised functions as there is no default implementation in impl<A: Iterator, B: Iterator> Iterator for Zip<A, B> even though it should just use the default in iterator.rs.

I have read the documentation and various posts but can not seem to find how to implement my desired behaviour above, would you know how?

As I was looking at writing a specialised nth_back() for TrustedRandomAccess. At the moment I would have to just copy and paste the default implementation across from iterator.rs for impl<A: Iterator, B: Iterator> Iterator for Zip<A, B> and only specialise in impl<A: TrustedRandomAccess + Iterator, B: TrustedRandomAccess + Iterator> Iterator for Zip<A, B>.

Just concerned about the implementations changing in iterator.rs and not being changed in zip.rs like it was for nth() now using advance_by() and not a loop, where as in zip.rs it is still using the older method of a loop.

@bors
Copy link
Contributor

bors commented Nov 23, 2020

☔ The latest upstream changes (presumably #79319) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@crlf0710
Copy link
Member

@DeveloperC286 Ping from triage! What's the current status of this?

@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 11, 2020
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@DeveloperC286
Copy link
Contributor Author

@DeveloperC286 Ping from triage! What's the current status of this?

Could not figure out how to clean it up as much as I wanted. However I think the PR I have now is an improvement so this is good for review now 👍

@DeveloperC286
Copy link
Contributor Author

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 18, 2020
@bors
Copy link
Contributor

bors commented Jan 16, 2021

☔ The latest upstream changes (presumably #81089) made this pull request unmergeable. Please resolve the merge conflicts.

@DeveloperC286
Copy link
Contributor Author

Hi @kennytm any chance you can take a look? I think my PR makes the code more simple/clean. Let me know your thoughts and I can just close the PR if you think it does not 👍

Copy link
Member

@kennytm kennytm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with nit

library/core/src/iter/adapters/zip.rs Outdated Show resolved Hide resolved
@kennytm
Copy link
Member

kennytm commented Jan 29, 2021

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Jan 29, 2021

📌 Commit 152f500 has been approved by kennytm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 29, 2021
henryboisdequin added a commit to henryboisdequin/rust that referenced this pull request Jan 29, 2021
…nnytm

refactor: removing custom nth Zip fn

Noticed `super_nth()` that seems very similar to `nth()` in `iterator.rs`.

If you look at `nth()` in `iterator.rs` before the commit `ecacc7534b6bf50205c37c89402565b82d95a257`  `super_nth()` looks exactly the same as `fn nth()` in `iterator.rs`.

I may be misunderstanding something, but I think `super_nth()` can just be removed.
@jonas-schievink
Copy link
Contributor

@bors r- failed in #81559 (comment) I believe

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 30, 2021
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 14, 2021
@crlf0710
Copy link
Member

crlf0710 commented Mar 5, 2021

@DeveloperC286 Ping from triage: Seems this pr is breaking other crates from compilation in the rollup above. Would you mind investigating into this? Thanks!

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 27, 2021

☔ The latest upstream changes (presumably #83580) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon
Copy link
Member

ping from triage:
@DeveloperC286 - can you please resolve the merge conflict and set the tag to S-waiting-on-author ? thank you.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@JohnCSimon
Copy link
Member

ping from triage:
@DeveloperC286 - fyi a build check is failing, can you post your status on this PR?

@DeveloperC286
Copy link
Contributor Author

ping from triage:
@DeveloperC286 - fyi a build check is failing, can you post your status on this PR?

Sorry struggled for a long time to reproduce the issue by bors in the roll up, but now the issue is present in the regular test suite.

Using the git bisect command the issue is with the commit refactor: removed ZipImpl, just do not understand the issue and trying to resolve it.

@rust-log-analyzer

This comment has been minimized.

When the iterator nth() method was updated it was not in zip.
Zip needs to implement nth() for the trusted length specialised implementation.
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-10 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling tracing-attributes v0.1.13
   Compiling rustc_macros v0.1.0 (/checkout/compiler/rustc_macros)
   Compiling chalk-derive v0.55.0
   Compiling chalk-ir v0.55.0
error[E0282]: type annotations needed for `(_, _)`
   --> /cargo/registry/src/github.com-1ecc6299db9ec823/serde_derive-1.0.125/src/de.rs:646:52
    |
646 |     let let_values = vars.clone().zip(fields).map(|(var, field)| {
    |                                                    ^^^^^^^^^^^^ consider giving this closure parameter the explicit type `(_, _)`, with the type parameters specified
    |
    = note: type must be known at this point
   Compiling tracing v0.1.25
error: aborting due to previous error

For more information about this error, try `rustc --explain E0282`.

@JohnCSimon
Copy link
Member

ping from triage:
@DeveloperC286 - can you please address the broken build, thanks.

@crlf0710
Copy link
Member

@DeveloperC286 I'm gonna close this due to inactivity. Feel free to reopen or create a new pr when you got time to work on this, thanks!

@crlf0710 crlf0710 closed this Jun 11, 2021
@crlf0710 crlf0710 added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-iterators Area: Iterators C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants